Instead of getAttribute, use the get modified attribute that is used in the effects. I don't think getAttribute would work here

Also

if ("Ancillary" and "Armor") in itemGroup:

I had no idea this was a syntax that worked... TIL

Also
if ("Ancillary" and "Armor") in itemGroup:
I had no idea this was a syntax that worked... TIL

There is some weirdness with it.

If you try:
if ("Armor" or "Shield") in itemGroup:
it will match True for Armor and False for Shield. Not as expected.

("Ancillary" and "Armor") in itemGroup
will match True against Small Ancillary Armor Repairer and false against Small Capacitor Booster II. As expected. If you put a breakpoint inside the if you will only land in there when you would expect to. (At least tested by throwing a bunch of modules on a fit and seeing which one is active when we hit the breakpoint.)

I did a bunch of testing against both and and or to make sure that I wasn't seeing things. It seems that and works fine. or does not. vOv

Instead of getAttribute, use the get modified attribute that is used in the effects. I don't think getAttribute would work here

You're right, we should use getModifiedItemAttr.

And you've probably noticed, but now getAttribute supports passing in a default value, and will now always return a None (before would return nothing). I modified ItemAttrShortcut and ChargeAttrShortcut the same way. I also wrapped all three in a try/except, because for some reason sometimes (but very rarely) we get thrown a AttributeError when trying to look up something that doesn't exist. 99% of the time we don't, but every now and again it happens (I ran into this a bunch implementing Gnosis into Eos, as we have a bazillion different things the same type of attribute can be called). This behavior should now match the built in Python getAttr() behavior, which as I understand it essentially does the same thing.

Just makes sense to me, but if you see issue with it....

I also cleaned up the logic. It was a bit messy before I wedged the extra logic in there, and was downright painful afterwards. Hopefully this is cleaner.

.

@blitzmann this should be ready to be merged.

Not within the context of discussion, but i think that if ("Ancillary" and "Armor") in itemGroup has misguiding syntax, implying that it checks that both items are in itemGroup, while actually it doesn't. As both of the items in braces are constants, this expression is superfluous and can be condensed. Evaluation of expression in braces will always return "Armor".

It works like: in case of "and" it evaluates 1st expression, if it's evaluated as True - evaluates 2nd, and so on. First seen expression evaluated as False will be returned, or last expression. Similar for "or" - if first expression is evaluated as False, 2nd will be evaluated. First seen expression evaluated as True will be returned, or last expression.

>>> ('1' or '2')
'1'
>>> ('' or '2')
'2'
>>> ('1' and '2')
'2'
>>> ('' and '2')
''

There're some applications for this logic, e.g. when you care about performance:

a = value or fall_back_call()

In this case, if value is evaluated as true, it will be assigned to a right away, without wasting resources to evaluate (rather expensive) function call. If value is evaluated as false, then function will be called and value returned by it will be assigned to a. This is unlike arguments in function calls, which are always evaluated and result of evaluation is passed to the function:

def pick_true_value(*args):
  for arg in args:
    if arg:
      return arg

a = pick_true_value(value, fall_back_call())

@DarkFenX: thanks for the clarification :) We should tweak the code to explicitly check for these to avoid problems.

@Ebag333: I am reluctant to modify the getModifiedItemAttr etc as these are core functions of eos, and I don't want to run into any unforeseen issues. Not to mention we already return None if the key doesn't exist, and current convention throughout pyfa for a deafult value has been to use item.getModifiedItemAttr('attr') or default.

Can you give me an instance to reproduce the AttributeError? Did you happen to dig deeper to figure out why it was happening?

@DarkFenX: thanks for the clarification :) We should tweak the code to explicitly check for these to avoid problems.

Yeah, playing with it further it doesn't always do what you would expect it to. Changed it to be explicit.

@Ebag333: I am reluctant to modify the getModifiedItemAttr etc as these are core functions of eos, and I don't want to run into any unforeseen issues. Not to mention we already return None if the key doesn't exist, and current convention throughout pyfa for a deafult value has been to use item.getModifiedItemAttr('attr') or default.
Can you give me an instance to reproduce the AttributeError? Did you happen to dig deeper to figure out why it was happening?

I can understand your reluctance, but it really shouldn't break anything. Anything that was expecting a None will still get a None returned (since default will default to None), we're just a bit more explicit about it. I was going to return 0 initially by default, but realized that potentially we have things that expect a None.

Things that use it like:
volley *= self.getModifiedItemAttr("damageMultiplier") or 1
Will still work the same, though it could be simplified down to:
volley *= self.getModifiedItemAttr("damageMultiplier", 1)

There's also a potential issue with that statement.

>>>None or 1
1
>>>2 or 1
2
>>>.5 or 1
0.5
>>>'Something' or 1
'Something'
>>>0 or 1
1

If the attribute is legitimately a 0, then it will return the or part of the statement, which means we're returning the wrong value (possibly, depending on what that expects). By changing it to returning a default rather than setting it via an or, we avoid that scenario (though that would take refactoring of a good number of the 2630 occurrences).

There are dozens if not hundreds of attributes that have a 0 value in the database (37948 to be precise). I can't even begin to tell you how many of those attributes would be used by the code, we might never run into it. But it is a potential issue, and I don't like having scenarios where we can return the wrong value.

As for reproducing the AttributeError....I'm afraid I don't remember exactly what I did when I was working on #812 but since that was 148 issues/PRs ago, a few brain cells might have been overwritten. :)

Either way it shouldn't hurt anything having the try/catch. Since for this usage AttributeError just means that it can't find that attribute, which we'd want to return None on anyway, it's just a bit more explicit.

/me edits

I can't even begin to tell you how many of those attributes would be used by the code

Sorry to quote myself, but I actually lied.

A quick regex query shows we use getModifiedItemAttr() and or a total of 43 times (we use getModifiedItemAttr a total of 2633 times). Most of them have the or as 0 or None. Most of the rest have the or as 1. (A few have the or set to another getModifiedItemAttr, which wouldn't change.)

For the most part, I don't see any scenarios where we run into the situation described above (can't think of a scenario where the attribute would be set to 0 for those particular attributes).

It's still a trap that a future dev could fall into, and a particularly subtle one that wouldn't be easily detected.

Most of them have the or as 0 or None. Most of the rest have the or as 1.

The cases in which we do or 1 are probably cases in which we know, if the value exists, it will return a non-0 value. But no, I don't really see an issue with it

I am still not sure why the try... except is there. The only way I see AttributeError happening here is if self was None, which shouldn't ever happen. I'm thinking that the error you experienced stemmed from inside the def __getitem__(self, key): of the ModifiedAttributeDict class.

I'm going to merge this in but revert the try... except - if it breaks, we need to know what exactly broke and why - if you experienced the error while working on the cap sim stuff, then it may be how you were accessing something, or doing it before values were available or something, because I don't believe we've ever run into an AttributeError there before. Let me know we details if it ever happens again. Not against adding a try there if needed, but the fact that we're trying to access something that doesn't exists gives me pause to just default it away.